Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link umockdev-record with libumockdev #148

Closed
wants to merge 1 commit into from
Closed

Conversation

martinpitt
Copy link
Owner

Avoid the duplicate static linking of the ioctl sources in
umockdev-record, and link it to libumockdev instead. The corresponding
symbols are already public in the C API anyway (as they match
src/umockdev.map), so just move the previously internal
classes/methods to public.

This also avoids confusing complaints about allegedly unused methods.

Fixes #138

Avoid the duplicate static linking of the ioctl sources in
umockdev-record, and link it to libumockdev instead. The corresponding
symbols are already public in the C API anyway (as they match
src/umockdev.map), so just move the previously `internal`
classes/methods to `public`.

This also avoids confusing complaints about allegedly unused methods.

Fixes #138
@martinpitt
Copy link
Owner Author

This can also be seen in https://salsa.debian.org/debian/umockdev/-/blob/master/debian/libumockdev0.symbols , all these supposedly-private symbols are there, like umockdev_ioctl_spi_recorder_construct(). If we want to avoid that, I think we rather need more fine-grained patterns in the map file.

@benzea, WDYT?

@benzea
Copy link
Collaborator

benzea commented Sep 12, 2021

Hmm, I didn't really want to make the path registration public and commit to the API there. Maybe not a big deal to have them as symbols though, as long as they are not documented.

@martinpitt
Copy link
Owner Author

This drives me nuts.. I tried with the below patch to make this API conditional on compiling the library and umockdev-record:

diff --git meson.build meson.build
index ecafcde..215b511 100644
--- meson.build
+++ meson.build
@@ -122,7 +122,7 @@ umockdev_lib = shared_library('umockdev',
     '-Wl,--no-undefined',
     '-Wl,--version-script,@0@/umockdev.map'.format(srcdir),
   ],
-  vala_args: [ '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
+  vala_args: [ '--define=INTERNAL_REGISTER_API', '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
   include_directories: include_directories('src'),
   version: lib_version,
   install: true,
@@ -173,7 +173,7 @@ umockdev_record_exe = executable('umockdev-record',
    'src/debug.c'],
   dependencies: [glib, gobject, gio_unix, vapi_posix, vapi_config, vapi_ioctl, libpcap],
   link_with: [umockdev_utils_lib],
-  vala_args: [ '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
+  vala_args: [ '--define=INTERNAL_REGISTER_API', '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
   include_directories: include_directories('src'),
   install: true)
 
diff --git src/umockdev-ioctl.vala src/umockdev-ioctl.vala
index a286faa..2d579e7 100644
--- src/umockdev-ioctl.vala
+++ src/umockdev-ioctl.vala
@@ -741,6 +741,7 @@ public class IoctlBase: GLib.Object {
           listeners.remove(devnode);
     }
 
+#if INTERNAL_REGISTER_API
     internal void register_path(GLib.MainContext? ctx, string devnode, string sockpath)
     {
         assert(DirUtils.create_with_parents(Path.get_dirname(sockpath), 0755) == 0);
@@ -782,6 +783,7 @@ public class IoctlBase: GLib.Object {
             });
         }
     }
+#endif // INTERNAL_REGISTER_API
 
     public virtual signal void client_connected(IoctlClient client) {
     }

But this doesn't work: If I leave out the --define for umockdev-record, the API (of course) does not exist:

FAILED: umockdev-record.p/src/umockdev-record.c umockdev-record.p/src/umockdev-ioctl.c umockdev-record.p/src/umockdev-pcap.c umockdev-record.p/src/umockdev-spi.c 
valac -C --debug --debug --pkg libpcap /var/home/martin/upstream/umockdev/src/ioctl.vapi /var/home/martin/upstream/umockdev/src/config.vapi --pkg posix --pkg gio-unix-2.0 --pkg gobject-2.0 --target-glib ' 2.32.0' --pkg glib-2.0 --color=always --directory umockdev-record.p --basedir ../ --vapidir=/var/home/martin/upstream/umockdev/src ../src/ioctl_tree.vapi ../src/umockdev-record.vala ../src/umockdev-ioctl.vala ../src/umockdev-pcap.vala ../src/umockdev-spi.vala umockdev-utils.vapi
../src/umockdev-record.vala:333.5-333.25: error: The name `register_path' does not exist in the context of `UMockdev.IoctlBase?'
    handler.register_path(null, dev, sockpath);
    ^^^^^^^^^^^^^^^^^^^^^
../src/umockdev-record.vala:479.9-479.30: error: The name `unregister_all' does not exist in the context of `UMockdev.IoctlBase?'
        handler.unregister_all();
        ^^^^^^^^^^^^^^^^^^^^^^
Compilation failed: 2 error(s), 0 warning(s)

But if I put in the define, it complains, even though umockdev-record clearly calls the API:

[1/6] valac -C --debug --debug --pkg libpcap /var/home/martin/upstream/umockdev/src/ioctl.vapi /var/home/martin/upstream/umockdev/src/config.vapi --pkg posix --pkg gio-unix-2.0 --pkg gobject-2.0 --target-glib ' 2.32.0' --pkg glib-2.0 --color=always --directory umockdev-record.p --basedir ../ --define=INTERNAL_REGISTER_API --vapidir=/var/home/martin/upstream/umockdev/src ../src/ioctl_tree.vapi ../src/umockdev-record.vala ../src/umockdev-ioctl.vala ../src/umockdev-pcap.vala ../src/umockdev-spi.vala umockdev-utils.vapi
../src/umockdev-ioctl.vala:772.5-772.33: warning: method `UMockdev.IoctlBase.unregister_path' never used
    internal void unregister_path(string devnode)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Compilation succeeded - 1 warning(s)

@martinpitt
Copy link
Owner Author

Oh, stupid me -- of course it really is not being used; umockdev-record only uses unregister_all(). I'll send a new PR, I have this working now.

@martinpitt martinpitt closed this Sep 14, 2021
@martinpitt martinpitt deleted the record-link-lib branch September 14, 2021 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unused `UMockdev.IoctlBase.unregister_{path,all}
2 participants